Hope you don't mind, but I think it's a well-known, slightly better approach.

Hey David,

Don't mind at all, in fact thank you for taking the time to point me in the right direction.

I've made some commits in accordance with your comments, so check it out and let me know if there is anything more I should do regarding this.

I noticed something that bothered me a bit but wasn't sure how to fix it—the horizontal alignment of the eye icon within the button doesn't seem quite centered; it's slightly shifted towards the top. It might not be a big deal, but I thought I'd point it out.

Testing pics:

Hey David,

I've went through your comments, I think I covered them all.

Nitpicking: what about using an internal component instead?

Here, I chose the second option, without the internal component. No particular reason, I've just went with the option which had a line of code less 😄 If you think that using internal component is a better choice, feel free to mention it and I'll update code accordingly.

No strong opinion from my side. Your choice is ok. Let's save lines of code until they are really needed 😉

Once you address them, we can continue working to go a a bit further 😇 is you're willing to do so 😉

Certainly, keep 'em coming. 😄

Great!

So, to be clear: from my point of view, the PR is ready to be merged as it is now. However, with very little effort, we can improve it a lot for extending that behavior to all password inputs. The idea is

• Create a src/components/core/PasswordInput.jsx component

  • And move the logic dealing with the password input and its reveal button there.

  • By now, I'd just forward received props to the internal TextInput but overriding the ones needed, like type. Something like

    export default function PasswordInput({ props }) {
       ...
    
      return (
         <>
           <TextInput {...props} type={type} />
      ...

• Add simple tests for that new component at src/components/core/PasswordInput.test.jsx file

  • Note that, although the philosophy of React Testing Library is about testing the application components in the way the user would use it, here we do not have a way to satisfy it (or at least I do not know how to do it 😜).

    Thus, instead of checking that the entered password is not in the screen before clicking the "Show Password" we will have to directly check the type attribute of the input before and after such an action.

    Definitely no in the way the user would use it, but looks like there are some caveats with the password input yet :) See [Role Parity] What do we do about input (type=password)? w3c/aria#935 and What do we recommend people do for elements that have no implicit role (like input[type=password]) testing-library/dom-testing-library#567.

    Something like below should work

    const passwordInput = screen.getByLabelText("Password");
    const button = screen.getByText("Show Password");
    
    expect(passwordInput).toHaveAttribute("type", "password");
    await user.click(button);
    expect(passwordInput).toHaveAttribute("type", "text")

• Replace all password inputs by the new PasswordInput component to make the UI more consistent and allow the user to reveal desired password, not only these related to a WiFi network.

  ❯ grep -rl "type=\"password" src/
  src/components/storage/iscsi/AuthFields.jsx
  src/components/questions/LuksActivationQuestion.jsx
  src/components/network/WifiConnectionForm.jsx
  src/components/core/PasswordAndConfirmationInput.jsx

• Check everything works

• Change the PR title

• Update the PR description

• Adapt the changes file

Do you accept the challenge?

Do you accept the challenge?

Of course, you can ping us at any moment you have doubts or need help. We can even jump into the PR for writing the unit test if you get stuck there.

Do you accept the challenge?

Accepted, I'll get on it. Also thanks for the pointers, they certainly help a lot.

I'm just not sure if using this widget makes sense in the first screenshot. The one with "password" and "password confirmation". It makes a lot of sense in all the others (iSCSI and WiFi), in which you are providing a previously set password. But I find combining a "password confirmation" with a password reveal to be weird.

Just my 2 cents about this topic. The "password confirmation" ensures you have typed the same in both inputs, and the "visibility icon" allows you to check you have typed what you really wanted. For me they are complementary. And IMHO, the "visibility icon" is even more important than the confirmation input. Reading the password is the safest way to check that I have really entered it correctly . In fact, having a confirmation is usually useless for me. Many times I write the password correctly but the confirmation wrongly. So having only the password input and the option to read it would be even faster and more secure for me.

And using autogenerated password would make the confirmation even more useless (you probably copy-paste in both inputs :-P).

It is only a reflection. I am not suggesting to do this ;-).

Believe me, I don't want to bother you with the code-review and suggestions. Far from it. However, I requested a new set of changes 😅 😅 😅

Haha, why not, glad to clean up 😄

I think I covered all suggested changes.
Regarding the unit tests where you believed they are probably tested as a part of components' library, I removed them.

Also, I addressed #760 while working on PasswordConfirmation component, so split prop is gone along with its wrapper.

Additionally, with the help of @joseivanlopez comment

I think the easiest way is to encrypt any device (disk or partition). If you are using a virtual machine, you can simply add a new disk (e.g., /dev/sdb) and then run the following command:

cryptsetup -y -v luksFormat /dev/sdb

After that, restart Agama (or change the product in the product selection page) and the encryption password popup should be shown.

Note: if you either skip or introduce the correct password, then the dialog is not shown anymore (you have to restart Agama to see it again).

I succeeded in reaching Luks Activation Question component and testing the password input.
Attached screenshots for this in PR description.

Thanks a lot for such a GREAT job

Don't mention it, anytime.

Added documentation comments and extracted the id prop.